Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stm32 i2c slave #2909

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

stm32 i2c slave #2909

wants to merge 14 commits into from

Conversation

jrmoulton
Copy link

This isn't quite finished. It only has the async slave implementation and the code needs to be cleaned up.

I could use some help debugging also.

In the listen method I enable the address match interrupt and register the waker in the poll closure.

The issue is that the runtime never polls the function again even when an address is matched.

If I write code that blocks instead of yielding and send and I2C communication the peripheral will still be enabled and will match the address.

I've tried also adding the wupen bit but that didn't make a difference

WUPEN:Wakeup from Stop mode enable 
0: Wakeup from Stop mode disable.
1: Wakeup from Stop mode enable.

Any idea why the runtime wouldn't poll the function again?

@bsodmike
Copy link
Contributor

bsodmike commented May 7, 2024

Hi Jared, interesting. Will respond soon if I figure anything out.

@jrmoulton
Copy link
Author

I've figured out why the function wasn't being polled. Now just working on finishing up the implementation

@jrmoulton
Copy link
Author

jrmoulton commented May 15, 2024

@Dirbaio the stm32 i2c supports multimaster for allowing both a master and a slave functionality. The slave mode is the default and the master mode is enabled by a start condition and ends with a stop condition.

Currently in this implementation I have split the slave from the master. This more closely matches the API in the embassy-rp crate. Would it be better to join them and merge the slave functionality into the regular i2c driver (in order to let you switch from slave to master without reconfiguring) or is it better to leave them split?

(edited)

@bsodmike
Copy link
Contributor

bsodmike commented May 16, 2024

I think being part of the regular driver works, if say you introduce a Option<Mode> type config option that is very obvious.

Thoughts?

@bsodmike
Copy link
Contributor

bsodmike commented Jun 1, 2024

@jrmoulton see CI issues please, just a heads up. keen to test this soon - thanks so much!!

edit: did some tracing to help you with picking this up mate - these seem CI related only, as your build works

@bsodmike
Copy link
Contributor

bsodmike commented Jun 1, 2024

@Dirbaio I like your CI page title "lol job"?? https://ci.embassy.dev/jobs/586c0e7a5b13

@bsodmike
Copy link
Contributor

bsodmike commented Jun 1, 2024

@jrmoulton see CI issues please, just a heads up. keen to test this soon - thanks so much!!

stm32_metapac::i2c::vals::Oamsk is there in the docs (v15.0.0)

test result: ok. 3 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 0.06s

    Updating crates.io index
    Updating git repository `https://github.com/embassy-rs/stm32-data-generated`
   Compiling embassy-hal-internal v0.1.0 (/ci/code/embassy-hal-internal)
   Compiling embassy-time v0.3.0 (/ci/code/embassy-time)
   Compiling embassy-usb-synopsys-otg v0.1.0 (/ci/code/embassy-usb-synopsys-otg)
   Compiling embassy-stm32 v0.1.0 (/ci/code/embassy-stm32)
   Compiling embassy-net-driver v0.2.0 (/ci/code/embassy-net-driver)
   Compiling embassy-embedded-hal v0.1.0 (/ci/code/embassy-embedded-hal)
error[E0432]: unresolved import `stm32_metapac::i2c::vals::Oamsk`
  --> src/i2c/mod.rs:19:5
   |
19 | use stm32_metapac::i2c::vals::Oamsk;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `Oamsk` in `i2c::vals`

warning: unused import: `interrupt`
  --> src/time_driver.rs:19:13
   |
19 | use crate::{interrupt, peripherals};
   |             ^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused import: `interrupt`
  --> src/exti.rs:14:13
   |
14 | use crate::{interrupt, pac, peripherals, Peripheral};
   |             ^^^^^^^^^

error[E0599]: no method named `init` found for struct `I2cSlave` in the current scope
   --> src/i2c/mod.rs:367:14
    |
299 | pub struct I2cSlave<'d, T: Instance, M: Mode> {
    | --------------------------------------------- method `init` not found for this struct
...
367 |         this.init(freq, config);
    |              ^^^^ method not found in `I2cSlave<'_, T, M>`

warning: unused variable: `waker`
   --> src/dma/ringbuffer.rs:429:33
    |
429 |         fn set_waker(&mut self, waker: &Waker) {}
    |                                 ^^^^^ help: if this is intentional, prefix it with an underscore: `_waker`
    |
    = note: `#[warn(unused_variables)]` on by default

_phantom: PhantomData,
};

this.init(freq, config);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing.

@@ -13,6 +16,7 @@ use embassy_hal_internal::{into_ref, Peripheral, PeripheralRef};
use embassy_sync::waitqueue::AtomicWaker;
#[cfg(feature = "time")]
use embassy_time::{Duration, Instant};
use stm32_metapac::i2c::vals::Oamsk;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Odd error in CI, needs a deeper look.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't been able to figure out why I am getting this error in CI.

Copy link
Contributor

@bsodmike bsodmike Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be @Dirbaio can help, he's very familiar with his CI setup. I'm curious too.

@jrmoulton
Copy link
Author

I'm in the process of reworking the I2C driver to be generic over an I2C mode which can either be Master, Slave, or MultiMaster.

@jkorinth
Copy link

Just wanted to let you know that I'm also eagerly awaiting slave mode, thanks a lot @jrmoulton!

@jrmoulton jrmoulton force-pushed the i2c-slave-new branch 3 times, most recently from 796b5c0 to b3dcfd4 Compare June 18, 2024 03:12
@jrmoulton jrmoulton marked this pull request as ready for review June 18, 2024 03:12
@jrmoulton jrmoulton marked this pull request as draft June 18, 2024 17:23
@jrmoulton jrmoulton marked this pull request as ready for review June 19, 2024 01:06
@jrmoulton
Copy link
Author

The last remaining ci issues are unrelated to this PR. Finally actually ready for review

@jrmoulton
Copy link
Author

@Dirbaio is there anything else I need to do in order for this PR to be reviewed?

@Dirbaio
Copy link
Member

Dirbaio commented Jun 25, 2024

yes, make CI pass. Once CI is green it'll be reviewed

@jrmoulton jrmoulton force-pushed the i2c-slave-new branch 4 times, most recently from 4da9bd1 to daeefdd Compare June 25, 2024 21:19
@okhsunrog
Copy link

any updates on this?

@jrmoulton
Copy link
Author

I need to undo a few changes that I made and then the CI should pass. It's close. I just haven't spent the last hour or two to get it finished.

@okhsunrog
Copy link

seems like now there are 2 pull requests for I2C slave on stm32. yesterday I tried to use I2C on stm32 and was confused which one to use.
#3196

@okhsunrog
Copy link

Can you please provide an example of using i2c slave for any stm32 mcu? @jrmoulton

@jrmoulton
Copy link
Author

I'll add one

@jrmoulton
Copy link
Author

I haven't actually tested the example that I've added. I've tested the slave code but not in this particular example. I've made this example match as closely as I could to the rp2040 i2c slave example.

@okhsunrog
Copy link

Thanks! I'll test the example with STM32L071CB tomorrow

@okhsunrog
Copy link

@Dirbaio can this be reviewed? It seem to look ready

@okhsunrog
Copy link

okhsunrog commented Aug 14, 2024

I tested the PR today, using STM32L071CB as slave and ESP32-C3 as master. I adapted your example, deleting master code, so only slave code it left. I'm looking at the transmission I see that time between read/write bit and ack from slave is about 280 us, so the clock gets stretched for 280 us. for some reason esp32-c3 doesn't like it. I see errors like:

E (44590) i2c.master: I2C transaction timeout detected
E (44590) i2c.master: s_i2c_synchronous_transaction(870): I2C transaction failed

(I set timeout to -1, I tried making it really big too, no result).
is there a way to decrease the gap? For now, when the slave sends ACK seems like the master is no longer listening, so the SDA stays pulled down and esp32-c3 just freezes. I guess, I should use different hardware as master or decrease the delay before ACK somehow?
image
image

@jrmoulton
Copy link
Author

@okhsunrog it looks like the esp32c3 doesn't support clock stretching. When you say you are increasing the timeout, do you mean on the esp32c3 master or on the stm32 slave?

https://docs.esp-rs.org/esp-hal/esp-hal/0.19.0/esp32c3/esp_hal/i2c/struct.I2C.html#method.new_with_timeout_async

@okhsunrog
Copy link

@jrmoulton it turned out that even the maximum timeout for SCL pin stretching wasn't enough, so I just patched esp-idf code to set I2C_TIME_OUT_EN bit to 0 manually, and now it doesn't time out. Otherwise I2C_TIME_OUT_INT interrupt is triggered. Now the code doesn't exit with timeout, esp32-c3 waits patiently for stm32 to answer. I'm just using i2c-tools from esp-idf examples for testing, with timeouts disabled by patching one line in esp-idf (they hardcoded setting this bit to 1).
Now, about slave code. Here's my code for STM32L071CBT6, it's as simple as possible: https://gist.github.com/okhsunrog/74ab86798a9bd5a91cf4328c5e56f67f
I'm inspecting the signal with Hantek + Pulseview software. Here's what I have:
on esp32c3 side sending works as extected:

i2c-tools> i2cset -c 0x22 -r 0xC0
I (649060) cmd_i2ctools: Write OK
i2c-tools> i2cget -c 0x22 -r 0xC0 -l 1
0x05 
i2c-tools> i2cset -c 0x22 -r 0xC0
I (659080) cmd_i2ctools: Write OK
i2c-tools> i2cset -c 0x22 -r 0xC0
I (663830) cmd_i2ctools: Write OK
i2c-tools> i2cget -c 0x22 -r 0xC0 -l 1
0x05 

on stm32 side I see strange timeouts:

0.000000 TRACE BDCR ok: 1c060003
└─ embassy_stm32::rcc::bd::{impl#3}::init @ /home/okhsunrog/.cargo/git/checkouts/embassy-cf3401b39709dec8/fc34291/embassy-stm32/src/rcc/bd.rs:196 
0.000000 DEBUG rcc: Clocks { hclk1: MaybeHertz(16000000), hsi: MaybeHertz(0), lse: MaybeHertz(0), lsi: MaybeHertz(0), pclk1: MaybeHertz(8000000), pclk1_tim: MaybeHertz(16000000), pclk2: MaybeHertz(8000000), pclk2_tim: MaybeHertz(16000000), rtc: MaybeHertz(32000), sys: MaybeHertz(32000000) }
└─ embassy_stm32::rcc::set_freqs @ /home/okhsunrog/.cargo/git/checkouts/embassy-cf3401b39709dec8/fc34291/embassy-stm32/src/rcc/mod.rs:71  
0.000152 INFO  Blinking LED, SDA, and SCL pins
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:77  
0.000274 INFO  Device start
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:80  
18.897460 INFO  Write command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:105 
19.897644 ERROR error while responding Timeout
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:110 
23.429931 INFO  Write command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:105 
24.430114 ERROR error while responding Timeout
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:110 
24.430358 INFO  Read command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:90  
24.430725 INFO  Successfully responded to read
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:96  
28.922821 INFO  Write command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:105 
29.923004 ERROR error while responding Timeout
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:110 
33.675384 INFO  Write command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:105 
34.675598 ERROR error while responding Timeout
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:110 
38.598083 INFO  Write command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:105 
39.598266 ERROR error while responding Timeout
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:110 
39.598510 INFO  Read command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:90  
39.598876 INFO  Successfully responded to read
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:96  
160.263458 INFO  Write command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:105 
161.263671 ERROR error while responding Timeout

And the I2C line stretching is really long for i2cget. For writing data, the SCL line is pulled only for about 270-280 us. When I do i2cget, the stretching for writing byte with "register address" is the same, about 280us, but for reading a byte of data, the stretching is really long! It's almost a second, 946ms. You can see on the screenshots below.
image
image
image

I found this in your code:

impl Default for Config {
    fn default() -> Self {
        Self {
            #[cfg(gpio_v2)]
            sda_pullup: false,
            #[cfg(gpio_v2)]
            scl_pullup: false,
            #[cfg(feature = "time")]
            timeout: embassy_time::Duration::from_millis(1000),
        }
    }
}

Maybe it's somehow related for the SCL line getting stretched for a whole second? And what exactly that timeout it about?

@jrmoulton
Copy link
Author

jrmoulton commented Aug 14, 2024

That timeout is used for the slave as a maximum time to wait for the master when doing a read or write. It isn't directly related.

The reason that the clock is being stretched for so long is that, the way it is written, the executor has to schedule the task that will respond to the request from the master. This code does not immediately start responding.

There are reasons that I wrote it this way. The biggest is that in order to respond immediately, the slave needs to have a buffer ready which doesn't really work if the slave can match multiple addresses. Some logic based on the matched address is necessary. I tried to find a way to store a closure to handle this logic but without an allocator I couldn't find a way to do it. So instead the clock is stretched until the task is scheduled and the user decides how to respond once they have seen the matched address.

It would probably be useful to have functions that work without clock stretching but I haven't written those

(Edit)
1 full second is extremely long though. I'm not sure what is going on there

@okhsunrog
Copy link

Yes, one second is very long. And what about ERROR error while responding Timeout? Master says that everything is okay, the signal looks fine, why the timeout then?
@jrmoulton

@jrmoulton
Copy link
Author

Not sure. There could be several reasons. Hard to say without seeing all of the code that you are using

@okhsunrog
Copy link

I'll send the full code tomorrow. I just got Discovery F4 board, I'll test it both as slave and as master and post the results here too

@okhsunrog
Copy link

@okhsunrog
Copy link

@jrmoulton yesterday I had some free time and I investigated into this issue some more time. The stretching of SCL for 1 second seems to be because of this:

impl Default for Config {
    fn default() -> Self {
        Self {
            #[cfg(gpio_v2)]
            sda_pullup: false,
            #[cfg(gpio_v2)]
            scl_pullup: false,
            #[cfg(feature = "time")]
            timeout: embassy_time::Duration::from_millis(1000),
        }
    }
}

I tried different timeouts manually and the scl is indeed stretched for the duration I set. When I set it to Duration::from_micros(100) the stretching is quite low, around 200-300 us, which is okay. The second issue is that your pub async fn respond_to_write function always return with timeout error, no matter which timeout I set, it never returns Ok. and the SCL line stretching time depends on the timeout value in config. could you look into this once again?

@okhsunrog
Copy link

@jrmoulton you're doing dma_transfer.await;, how does it work? when does this feature gets polled?
When I replace the code timeout.with(self.read_dma_internal_slave(buffer, timeout)).await with self.read_dma_internal_slave(buffer, timeout).await the code gets stuck forever. What could be the issue?

@okhsunrog
Copy link

Did you test both blocking and async?

@bsodmike
Copy link
Contributor

bsodmike commented Sep 3, 2024

That's the code I used for slave: https://github.com/okhsunrog/stm32l071_i2c_slave_test

That's the code I used for master: https://github.com/espressif/esp-idf/tree/release/v5.3/examples/peripherals/i2c/i2c_tools

The only difference is that in this line: https://github.com/espressif/esp-idf/blob/6568f8c553f89c01c101da4d6c735379b8221858/components/hal/esp32c3/include/hal/i2c_ll.h#L191 I replaced 1 with 0

Can you share your memory.x as well please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants